Skip to content

Trap on sync stream/future cancel and subtask.cancel when in a waitable set#13708

Open
GodlyDonuts wants to merge 1 commit into
bytecodealliance:mainfrom
GodlyDonuts:async-cancel-waitable-set-trap
Open

Trap on sync stream/future cancel and subtask.cancel when in a waitable set#13708
GodlyDonuts wants to merge 1 commit into
bytecodealliance:mainfrom
GodlyDonuts:async-cancel-waitable-set-trap

Conversation

@GodlyDonuts

@GodlyDonuts GodlyDonuts commented Jun 22, 2026

Copy link
Copy Markdown

Fixes #13690.

component-model#647 tightened the trap rules for synchronous stream/future
operations: a synchronous read/write must trap if the waitable it operates
on has already been added to a waitable set. That PR extended the same rule to
{stream,future}.cancel-{read,write} and subtask.cancel, but Wasmtime only
implemented it for read/write. As a result the upstream
trap-if-sync-and-waitable-set.wast test hits an unreachable on the cancel
cases today instead of trapping.

Why it was missing

The check lives in wait_for_event, the single point every blocking
synchronous read/write funnels through before suspending:

if waitable.common(state)?.set.is_some() {
    bail!(Trap::WaitableSyncAndAsync);
}

The cancel paths don't reliably reach it. When there is nothing pending to wait
on, cancel_read/cancel_write take an early return with a Cancelled code,
and subtask.cancel can resolve a starting/finished subtask without ever
suspending. In those cases the waitable-set check was never run, so the cancel
succeeded where the spec requires a trap.

Fix

Pull the check into a small helper on Waitable:

fn trap_if_in_waitable_set(&self, state: &mut ConcurrentState) -> Result<()> {
    if self.common(state)?.set.is_some() {
        bail!(Trap::WaitableSyncAndAsync);
    }
    Ok(())
}

wait_for_event now calls it (no behavior change), and the three synchronous
cancel entry points — cancel_read, cancel_write, and subtask_cancel
call it up front, guarded by !async_ to match the neighbouring
check_blocking guards. The async variants are deliberately untouched: being a
member of a waitable set is the whole point of an asynchronous cancel.

Testing

Added tests/misc_testsuite/component-model/async/cancel-sync-and-waitable.wast
covering all five operations (future.cancel-{read,write},
stream.cancel-{read,write}, and subtask.cancel). Each starts a pending
operation, joins the waitable to a fresh set, and asserts the synchronous
cancel traps; this mirrors the upstream trap-if-sync-and-waitable-set.wast. I
verified each case regresses (returns a cancel code / completes normally rather
than trapping) when its individual guard is removed.

Depends on bytecodealliance/wit-bindgen#1638

Enforcing this trap surfaces a latent bug in wit-bindgen's async runtime:
cancel_inter_task_stream_read issues a synchronous stream.cancel-read on its
inter-task wakeup stream while that stream is still in the task's waitable set
(it calls remove_waitable only afterwards). That is exactly the pattern this
trap now (correctly) rejects, so every wasi:http@0.3.0 (p3) program built with
the current wit-bindgen traps during ordinary operation — which is what the
red p3 CI jobs here are.

bytecodealliance/wit-bindgen#1638 reorders that to leave the set before
cancelling, matching the unregister-then-cancel ordering the general
WaitableOperation::cancel path already uses. With that change applied locally
(via [patch.crates-io]), the full p3 suites pass again:

  • wasmtime-wasi-http p3: 25 passed
  • wasmtime-wasi p3: 27 passed
  • component-model/async wast (incl. the new test): 192 passed

So this PR needs a wit-bindgen release containing #1638 and a corresponding
version bump before its p3 jobs go green. Happy to fold the bump in here once
that's available, or sequence it however you prefer.

…le set

Synchronous `{stream,future}.{read,write}` already trap when their waitable
has been added to a waitable set, but the corresponding `cancel-{read,write}`
and `subtask.cancel` intrinsics did not, even though component-model#647 added
the same trap for them.

The existing trap is enforced in `wait_for_event`, which every blocking sync
read/write funnels through. The sync cancel paths don't always reach it: when
there is nothing pending to wait on, `cancel_read`/`cancel_write` return a
`Cancelled` code directly and `subtask.cancel` resolves without suspending, so
the "in a waitable set" check was simply never performed.

Hoist that check into a small `Waitable::trap_if_in_waitable_set` helper and
call it from the three sync cancel entry points (guarded by `!async_`, like the
neighbouring `check_blocking` calls). `wait_for_event` now uses the same helper
so the single source of truth is shared.

Adds a regression test covering all five operations, mirroring the upstream
`trap-if-sync-and-waitable-set.wast` spec test.
@GodlyDonuts GodlyDonuts requested a review from a team as a code owner June 22, 2026 17:54
@GodlyDonuts GodlyDonuts requested review from cfallin and removed request for a team June 22, 2026 17:54
@cfallin cfallin requested review from dicej and removed request for cfallin June 22, 2026 18:03
@cfallin

cfallin commented Jun 22, 2026

Copy link
Copy Markdown
Member

I'm going to redirect review to someone who knows much more about the CM async machinery (hope that's ok @dicej!).

@dicej dicej left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

CI is failing, and it seems to point to wit_bindgen::rt's internal machinery for handling inter-task communication. I suspect a change will be needed in that project in order to get those tests passing. I'm on vacation at the moment (and should not have opened my computer at all 😅 ), but I can take a closer look when I get back if nobody beats me to it.

@GodlyDonuts

GodlyDonuts commented Jun 22, 2026

Copy link
Copy Markdown
Author

Thanks @dicej! That matches what I found, the trap fires in wit-bindgen's cancel_inter_task_stream_read, which issues a synchronous stream.cancel-read on the inter-task wakeup stream while it's still in the task's waitable set (it calls remove_waitable only afterwards).

I opened bytecodealliance/wit-bindgen#1638 to fix that: it reorders the two so the stream leaves the waitable set before the synchronous cancel, matching the unregister-then-cancel ordering the general WaitableOperation::cancel path already uses. With that patched in locally ([patch.crates-io]), the full p3 suites pass again, wasmtime-wasi-http p3 (25), wasmtime-wasi p3 (27), and component-model/async wast incl. the new test (192).

So this just needs a wit-bindgen release containing #1638 and the corresponding bump here. Happy to fold the bump into this PR once it's available, no rush, enjoy your vacation. 🙂

@GodlyDonuts

Copy link
Copy Markdown
Author

wit-bindgen#1638 is merged, thanks @alexcrichton. Only thing left here is a wit-bindgen release that carries it plus a matching version bump in this PR. I'll put the bump up as soon as there's something on crates.io to point at. Verified locally that the p3 suites go green with the fix in, so CI should be clean once it's bumped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

wasmtime:api Related to the API of the `wasmtime` crate itself

Projects

None yet

Development

Successfully merging this pull request may close these issues.

async: missing "in waitable set" trap in sync {stream,future}.cancel-{read,write} and subtask.cancel

3 participants